-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Datasources - Part 1: Rework datasource classes #3101
Conversation
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
This is the first part of the division of the PR #2959 |
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@Override | ||
public String getBaseName() { | ||
return baseName; | ||
DirectoryDataSource(Path directory, String baseName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add also
DirectoryDataSource(Path directory, String baseName, CompressionFormat compressionFormat)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it wouldn't be used so I'm not sure we should add it.
* @return true if the file exists, else false | ||
*/ | ||
@Override | ||
public boolean exists(String suffix, String ext) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to AbstractFileSystemDatasource and remove the same method from DirectoryDataSource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
…ataSource Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
|
||
UnsupportedOperationException exception = assertThrows(UnsupportedOperationException.class, () -> { | ||
try (OutputStream ignored = dataSource.newOutputStream("foo.bar", true)) { | ||
fail(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to fail explicitly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed explicitly but it need something in the try() that cannot throw a runtime exception
|
||
static Stream<Arguments> provideArgumentsForWriteThenReadTest() { | ||
return Stream.of( | ||
Arguments.of("foo", "iidm", CompressionFormat.ZIP), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use the same semantics for the "mainExtension" as other test classes
Arguments.of("foo", "iidm", CompressionFormat.ZIP),
vs
Arguments.of("foo", ".iidm", CompressionFormat.ZSTD),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or just remove the mainExtension concept from this PR entirely, it's just used for tests and you can put everything in one string)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
static Stream<Arguments> provideArgumentsForClassAndListingTest() { | ||
return Stream.of( | ||
Arguments.of("foo.iidm", CompressionFormat.BZIP2, Bzip2DirectoryDataSource.class, | ||
Set.of("foo", "foo.txt", "foo.iidm", "foo.xiidm", "foo.v3.iidm", "foo.v3", "foo_bar.iidm", "foo_bar", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe factorize the sets as they are the same? which would highlight the fact that so far the listed files are the same for the 3 parameterized tests.
Same comment for other test classes.
protected Path testDir; | ||
protected Set<String> unlistedFiles; | ||
protected Set<String> existingFiles; | ||
protected ArchiveFormat archiveFormat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this parameter should rather be in AbstractArchiveDataSourceTest
|
||
// Create an archive when needed | ||
if (archiveFormat != null) { | ||
createArchiveAndFiles(fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should rather be in the setup of AbstractArchiveDataSourceTest
Quality Gate passedIssues Measures |
* reorganize datasource classes * add tests Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
No
What kind of change does this PR introduce?
Feature
What is the current behavior?
All the existing
DataSources
based on files extendFileDataSources
. The difference between datasources considering files in a directory and the datasources considering files in an archive is not clear enough.What is the new behavior (if this is a feature change)?
Two types of
DataSources
based on files now exists:DirectoryDataSource
(inherited byGzDirectoryDataSource
,Bzip2DirectoryDataSource
,ZstdDirectoryDataSource
,XZDirectoryDataSource
): considers files in a directoryAbstractArchiveDataSource
(onlyZipArchiveDataSource
for now but aTarArchiveDataSource
could be added later): considers files in an archive.Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Two types of
DataSources
based on files now exists:DirectoryDataSource
(inherited byGzDirectoryDataSource
,Bzip2DirectoryDataSource
,ZstdDirectoryDataSource
,XZDirectoryDataSource
): considers files in a directoryAbstractArchiveDataSource
(onlyZipArchiveDataSource
for now but aTarArchiveDataSource
could be added later): considers files in an archive.Therefore, multiple classes have changed, please update your code if you used them directly:
Bzip2FileDataSource
Bzip2DirectoryDataSource
FileDataSource
DirectoryDataSource
GzFileDataSource
GzDirectoryDataSource
XZFileDataSource
XZDirectoryDataSource
ZipFileDataSource
ZipArchiveDataSource
ZstdFileDataSource
ZstdDirectoryDataSource
Note: Their constructors are not the same so you will have to adapt your code to the new ones, depending on which DataSource you use.
Other information: